Add isConsistentReadGuaranteed() to StorageInfo #3201
Conversation
Summary of ChangesHello @brfrn169, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new isConsistentReadGuaranteed() method to the StorageInfo interface and renames MutationAtomicityUnit to the more general AtomicityUnit. The changes are well-implemented and consistently applied across the codebase. The dynamic determination of consistency guarantees for JDBC based on transaction isolation levels is a particularly nice improvement. I have a couple of minor suggestions for consistency and clarity.
...a/com/scalar/db/storage/objectstorage/ObjectStorageMutationAtomicityUnitIntegrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new method isConsistentReadGuaranteed() to the StorageInfo interface to indicate whether a storage backend guarantees consistent reads within its atomicity unit. It also refactors the naming from MutationAtomicityUnit to AtomicityUnit to reflect that atomicity applies to all operations, not just mutations.
Key changes:
- Added
isConsistentReadGuaranteed()method to indicate read consistency guarantees - Renamed
MutationAtomicityUnitenum toAtomicityUnitand updated all references - Implemented JDBC-specific logic to dynamically determine consistent read guarantees based on database isolation levels
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
StorageInfo.java |
Added isConsistentReadGuaranteed() method and renamed enum from MutationAtomicityUnit to AtomicityUnit |
StorageInfoImpl.java |
Added consistentReadGuaranteed field and updated constructor, equals, hashCode, and toString methods |
DistributedStorageAdmin.java |
Added warning about internal use of getStorageInfo() method |
DistributedStorage.java |
Updated Javadoc references from MutationAtomicityUnit to AtomicityUnit |
| Various storage admin implementations | Updated to pass consistentReadGuaranteed parameter (Cassandra, Cosmos, Dynamo, ObjectStorage) |
RdbEngineStrategy.java |
Added getMinimumIsolationLevelForConsistencyRead() method interface |
| Various RDB engine implementations | Implemented getMinimumIsolationLevelForConsistencyRead() for each database engine |
JdbcAdmin.java |
Dynamically determines consistentReadGuaranteed based on connection isolation level |
| Test files | Updated test method names and enum references from MutationAtomicityUnit to AtomicityUnit |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineMysql.java
Outdated
Show resolved
Hide resolved
brfrn169
force-pushed
the
add-isConsistentReadGuaranteed-to-StorageInfo
branch
from
69c0c67 to
4e9a18e
Compare
|
@brfrn169 I'm confused about the definition of |
|
Since I need to revise the changes based on the discussion, I will close this PR. |
2 participants
Description
This PR introduces a new method
isConsistentReadGuaranteed()method to theStorageInfointerface to indicate whether a storage backend guarantees consistent reads within its atomicity unit. It also renamesMutationAtomicityUnitto the more generalAtomicityUnitto better reflect that atomicity applies to all operations, not just mutations.Implementation:
true(guarantees consistent reads within partitions)true(guarantees consistent reads within partitions)false(currently does not guarantee consistent reads as Scan and Query support READ_COMMITTED isolation level)trueif isolation level is REPEATABLE READ or SERIALIZABLEtrueif isolation level is REPEATABLE READ or SERIALIZABLEtrueif isolation level is SERIALIZABLEtrueif isolation level is REPEATABLE READ or SERIALIZABLEtrueif isolation level is SERIALIZABLEtrueif isolation level is REPEATABLE READ or SERIALIZABLERelated issues and/or PRs
N/A
Changes made
StorageInfo#isConsistentReadGuaranteed()method to indicate whether a storage backend guarantees consistent reads within its atomicity unitStorageInfo.MutationAtomicityUnitenum toStorageInfo.AtomicityUnitto reflect that atomicity applies to both reads and mutationsgetMutationAtomicityUnit()togetAtomicityUnit()in theStorageInfointerfaceDistributedStorageAdmin#getStorageInfo()that this feature is primarily for internal use and subject to breaking changesChecklist
Additional notes (optional)
N/A
Release notes
Added
isConsistentReadGuaranteed()method to theStorageInfointerface to indicate whether a storage backend guarantees consistent reads within its atomicity unit. Also renamed MutationAtomicityUnit to AtomicityUnit to better reflect that atomicity applies to all operations, not just mutations.